Drop wrappers from Internal Server #536

maneeshpm · 2021-05-22T19:58:21Z

This is the first step, fixes #430

The aim of this step is to internally remove usage of wrapper structures in libkiwix as much as possible. A majority of work in this step will be focused around bringing this change in the internal server which will in turn add new methods/change existing code to use libzim structures such as zim::Archive, zim::Searcher, zim::SearchResultSet etc directly instead of adding additional layer of kiwix::Reader or kiwix::Searcher.

Changes included in this PR:

Drop wrappers from InternalServer completely
Add libzim structure mirror members to Library and other kiwix structures
Add new method in SearchRenderer to use libzim structures directly
Move helpers for handling archive to archiveTools

include/library.h

kelson42 · 2021-05-30T06:30:43Z

@maneeshpm Thx for this important PR!

src/server/internalServer.cpp

include/library.h

src/server/internalServer.cpp

include/search_renderer.h

kelson42 · 2021-06-16T04:10:28Z

@maneeshpm @mgautierfr This PR ahould be rebased ans reviwed again,

mgautierfr

This is far better.

Still some code to change but nothing fundamental or complex to change.

src/library.cpp

include/tools/archiveTools.h

include/tools/stringTools.h

src/tools/archiveTools.cpp

codecov · 2021-06-16T18:43:44Z

Codecov Report

Merging #536 (6f63914) into master (0594e60) will decrease coverage by 0.35%.
The diff coverage is 82.02%.

@@            Coverage Diff             @@
##           master     #536      +/-   ##
==========================================
- Coverage   65.26%   64.91%   -0.36%     
==========================================
  Files          52       53       +1     
  Lines        3648     3737      +89     
  Branches     1840     1884      +44     
==========================================
+ Hits         2381     2426      +45     
- Misses       1265     1309      +44     
  Partials        2        2

Impacted Files	Coverage Δ
include/library.h	`91.66% <ø> (ø)`
include/reader.h	`88.88% <ø> (ø)`
include/search_renderer.h	`100.00% <ø> (ø)`
include/searcher.h	`33.33% <ø> (-66.67%)`	⬇️
include/tools/stringTools.h	`50.00% <ø> (ø)`
src/searcher.cpp	`47.10% <0.00%> (-9.20%)`	⬇️
src/server/internalServer.h	`100.00% <ø> (ø)`
src/search_renderer.cpp	`90.00% <76.19%> (-10.00%)`	⬇️
src/server/internalServer.cpp	`83.61% <78.09%> (-2.49%)`	⬇️
src/library.cpp	`79.20% <83.33%> (+0.08%)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0594e60...6f63914. Read the comment docs.

maneeshpm · 2021-06-16T19:02:53Z

@kelson42 Rebase done. @mgautierfr The last three commits are the new changes.

mgautierfr

Just a small change to correctly handle path=="/" and we should be good.

src/tools/archiveTools.cpp

kelson42 · 2021-06-23T17:07:08Z

@mgautierfr Can you please review and have a look why the ci fails, this is since las week like this.

maneeshpm · 2021-06-23T17:37:27Z

@kelson42 I reran the jobs, now the ci is passing. There is only the coverage issue, which we can expect since we have removed the usage of readers and searchers from internal server.

kelson42 · 2021-06-23T17:44:52Z

@maneeshpm Nice, then you probably just need to write one or two additional tests and we are good

kelson42 · 2021-06-25T06:36:30Z

I have rebased on master

maneeshpm · 2021-06-25T11:42:43Z

@kelson42 I have added the tests and ci is passing now 😄 .
Since we will be dropping more usage of Reader and Searcher from our codebase in the subsequent PRs, the Reader tests I added now somewhat demonstrates how its simply a wrapper on an Archive and how we can use an archive directly. I will add more tests in the coming PRs that will set us up for removing the wrappers completely.

test/reader.cpp

These members will mirror the functionality offered by equivalent usage of Reader class.

This is essentially a code move of meta handlers from using Reader functions to directly using Archive.

Introduces a new member mp_search that houses the zim::Search object, adds a new constructor for this purpose. This commit also add an overload for getHtml that takes start and end integers as arguments since they are not part of the search object we include.

Since we now have SearcherRenderer that can work with native libzim structure, we will drop the wrapper and use them instead.

Removing the functions in InternalServer that are no longer needed.

Even though we will be removing the wrappers soon, the test coverage should be complete and we could simply remove these files later.

mgautierfr · 2021-07-06T13:53:42Z

@maneeshpm Do you want to adapt the test now the libzim pr is merged ? (once the nightly is passed)
Or we merge now ? (it is not so important to change this as the test will be removed "soon")

maneeshpm · 2021-07-06T13:58:51Z

@mgautierfr I expect the following PR to be a small one(and that depended on the libzim pr). So we can go ahead with merging this one :D

maneeshpm commented May 23, 2021

View reviewed changes

include/library.h Show resolved Hide resolved

maneeshpm force-pushed the internally_drop_reader_searcher branch 5 times, most recently from 3183026 to 90e701a Compare May 28, 2021 08:32

maneeshpm marked this pull request as ready for review May 28, 2021 08:33

maneeshpm changed the title ~~[WIP] Internally drop reader searcher~~ Drop wrappers from Internal Server May 28, 2021

maneeshpm requested a review from mgautierfr May 29, 2021 20:04

kelson42 linked an issue May 30, 2021 that may be closed by this pull request

Drop libzim wrapper #430

Closed

mgautierfr requested changes May 31, 2021

View reviewed changes

kelson42 force-pushed the internally_drop_reader_searcher branch from bddd141 to b24f46c Compare June 4, 2021 06:43

maneeshpm force-pushed the internally_drop_reader_searcher branch from b24f46c to 7c572ec Compare June 4, 2021 15:36

maneeshpm requested a review from mgautierfr June 4, 2021 15:52

kelson42 removed a link to an issue Jun 10, 2021

Drop libzim wrapper #430

Closed

mgautierfr requested changes Jun 16, 2021

View reviewed changes

maneeshpm force-pushed the internally_drop_reader_searcher branch from 7c572ec to 9eb9517 Compare June 16, 2021 18:41

maneeshpm requested a review from mgautierfr June 16, 2021 19:03

mgautierfr requested changes Jun 17, 2021

View reviewed changes

src/tools/archiveTools.cpp Outdated Show resolved Hide resolved

maneeshpm force-pushed the internally_drop_reader_searcher branch 2 times, most recently from 982adde to c31c5ed Compare June 19, 2021 14:17

maneeshpm requested a review from mgautierfr June 20, 2021 03:53

kelson42 force-pushed the internally_drop_reader_searcher branch from c31c5ed to 435c6ad Compare June 25, 2021 06:36

maneeshpm mentioned this pull request Jun 27, 2021

[WIP] Extend libkiwix structures to use libzim #570

Closed

mgautierfr approved these changes Jun 30, 2021

View reviewed changes

test/reader.cpp Show resolved Hide resolved

mgautierfr mentioned this pull request Jun 30, 2021

Add string cast to zim::Uuid openzim/libzim#581

Closed

maneeshpm force-pushed the internally_drop_reader_searcher branch 2 times, most recently from 49d6def to c469a7f Compare July 3, 2021 04:34

maneeshpm added 9 commits July 3, 2021 14:02

Add m_archives and getArchiveById to Library

940368b

These members will mirror the functionality offered by equivalent usage of Reader class.

Drop usage of Reader from InternalServer::handle_meta

7d68926

This is essentially a code move of meta handlers from using Reader functions to directly using Archive.

Drop usage of Reader from InternalServer::handle_suggest

a236751

Drop Reader from InternalServer::handle_random

75b4d31

Drop Reader and Entry wrappers from handle_content

c046f64

Drop wrapper structures from handle_search

bc82163

Since we now have SearcherRenderer that can work with native libzim structure, we will drop the wrapper and use them instead.

Remove unwanted reader functions

a94a03c

Removing the functions in InternalServer that are no longer needed.

Add unit tests for Searcher and Reader

6f63914

Even though we will be removing the wrappers soon, the test coverage should be complete and we could simply remove these files later.

maneeshpm force-pushed the internally_drop_reader_searcher branch from c469a7f to 6f63914 Compare July 3, 2021 08:38

maneeshpm mentioned this pull request Jul 3, 2021

Extend libkiwix structures to be contructed/updated from libzim structures #576

Merged

mgautierfr merged commit 1c0b450 into master Jul 6, 2021

mgautierfr deleted the internally_drop_reader_searcher branch July 6, 2021 14:18

maneeshpm mentioned this pull request Jul 9, 2021

Iterative searcher #166

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop wrappers from Internal Server #536

Drop wrappers from Internal Server #536

maneeshpm commented May 22, 2021 •

edited

Loading

kelson42 commented May 30, 2021

kelson42 commented Jun 16, 2021

mgautierfr left a comment

codecov bot commented Jun 16, 2021 •

edited

Loading

maneeshpm commented Jun 16, 2021

mgautierfr left a comment

kelson42 commented Jun 23, 2021

maneeshpm commented Jun 23, 2021

kelson42 commented Jun 23, 2021

kelson42 commented Jun 25, 2021 •

edited

Loading

maneeshpm commented Jun 25, 2021

mgautierfr commented Jul 6, 2021

maneeshpm commented Jul 6, 2021 •

edited

Loading

Drop wrappers from Internal Server #536

Drop wrappers from Internal Server #536

Conversation

maneeshpm commented May 22, 2021 • edited Loading

kelson42 commented May 30, 2021

kelson42 commented Jun 16, 2021

mgautierfr left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 16, 2021 • edited Loading

Codecov Report

maneeshpm commented Jun 16, 2021

mgautierfr left a comment

Choose a reason for hiding this comment

kelson42 commented Jun 23, 2021

maneeshpm commented Jun 23, 2021

kelson42 commented Jun 23, 2021

kelson42 commented Jun 25, 2021 • edited Loading

maneeshpm commented Jun 25, 2021

mgautierfr commented Jul 6, 2021

maneeshpm commented Jul 6, 2021 • edited Loading

maneeshpm commented May 22, 2021 •

edited

Loading

codecov bot commented Jun 16, 2021 •

edited

Loading

kelson42 commented Jun 25, 2021 •

edited

Loading

maneeshpm commented Jul 6, 2021 •

edited

Loading